Skip to content

Conversation

@SylvainJuge
Copy link
Contributor

@SylvainJuge SylvainJuge commented Sep 12, 2025

Part of #13031 for reactor

@SylvainJuge SylvainJuge self-assigned this Sep 12, 2025
@SylvainJuge SylvainJuge marked this pull request as ready for review September 12, 2025 11:02
@SylvainJuge SylvainJuge requested a review from a team as a code owner September 12, 2025 11:02
@Advice.Argument(1)
BiConsumer<? super HttpClientResponse, ? super Throwable> originalResponseCallback) {

// intermediate variables needed for inlined instrumentation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was there some side effect that surfaced that caused issues with referencing the originals without making an intermediate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when removing those this makes the tests fail with missing spans. I remember having seen this elsewhere and thus applied this workaround after spending too much time doing diffs and partial reverts. It could be something worth investigating further as it might be related to bytebuddy or the way we use it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as far as I remember byte-buddy does not allow you to reassign a value to a read only argument

Comment on lines +81 to +82
* Dedicated advice scope subclass that make instrumentation skip original method body using
* {@code skipOn = Runnable.class } which does not require to expose an extra type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bit of a hack, but overall should be nicer than the alternatives

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, also it's quite rare (and probably the only instance) that we have to dynamically skip original method body while also overriding/wrapping arguments.

@trask trask enabled auto-merge (squash) September 15, 2025 16:05
@trask trask merged commit ecdaa63 into open-telemetry:main Sep 15, 2025
89 checks passed
@SylvainJuge SylvainJuge deleted the indy-reactor-2 branch September 15, 2025 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants